trident-acl-agent: rewrite as annotation-based ACL update agent by bfjelds · Pull Request #730 · microsoft/trident

bfjelds · 2026-08-05T19:35:48Z

Summary

trident-acl-agent implements annotation-based design: watching Node's acl.azure.com/update-request annotation, driving update stage/finalize, rollback stage/finalize, and commit gRPC operations against tridentd, and writing acl.azure.com/update-status.

Following design found here: https://msazure.visualstudio.com/One/_git/Compute-ACL-Update-Service?version=GC67946fff8f296e10217b70e063c896e6028ea843&path=/docs/update-trigger-design.md

Context

This is the second step in enabling trident-acl-agent to run updates and rollbacks. Related PRs:

Validation

First 2 PRs (checking for RPM build mostly): https://dev.azure.com/mariner-org/ECF/_build/results?buildId=1176233&view=results
Validate trident-acl-agent implementation with full trident-acl-agent tests: https://dev.azure.com/mariner-org/ECF/_build/results?buildId=1176259&view=results

PR details

annotations.rs — UpdateRequest/UpdateStatus wire types, schema validation, and design-doc conformance tests (parses the request/status JSON examples from docs/update-trigger-design.md with the real (de)serialization code, and validates both those examples and our own constructed annotations against the formal JSON Schema embedded in that document).
orchestrator.rs — the reconcile loop: stage/finalize/rollback handlers, the post-reboot commit resume/reconstruction path, and terminal status-mapping logic (with unit tests against a mock tridentd).
trident.rs — gRPC client wrapper for tridentd's stable v1 Update/Commit/Rollback services.
mock_tridentd.rs — in-process fake tridentd server (dev-only) used by orchestrator.rs's unit tests.
k8s.rs — thin kube-rs wrapper for watching/patching the agent's own Node.
config.rs, state.rs — agent configuration and the persisted state file.

Packaging

Adds the trident-acl-agent.service systemd unit and RPM spec entries so the agent ships and starts on ACL images.

azure-pipelines · 2026-08-05T19:35:57Z

Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run.

Rewrites trident-acl-agent from the earlier label-protocol prototype into the accepted annotation-based design: it watches a Node's acl.azure.com/update-request annotation, drives Trident's stage/finalize/ rollback/commit gRPC operations against tridentd, and writes back acl.azure.com/update-status, including the post-reboot commit half of a finalize/rollback via a small persisted state file (/var/lib/trident-acl-agent/state.json). New modules: - annotations.rs: UpdateRequest/UpdateStatus wire types, schema validation, and design-doc conformance tests (parses the request/status JSON examples from docs/update-trigger-design.md with the real (de)serialization code, and validates both those examples and our own constructed annotations against the formal JSON Schema embedded in that document). - orchestrator.rs: the reconcile loop - stage/finalize/rollback handlers, the post-reboot commit resume/reconstruction path, and the terminal status-mapping logic (including its unit tests against a mock tridentd). - trident.rs: gRPC client wrapper for tridentd's stable v1 Update/Commit/ Rollback services. - mock_tridentd.rs: in-process fake tridentd server (dev-only) used by orchestrator.rs's unit tests. - k8s.rs: thin kube-rs wrapper for watching/patching the agent's own Node. - config.rs, state.rs: agent configuration and the persisted state file. Depends on the stable RollbackService promoted in the parent branch (user/bfjelds/rollback-grpc-promotion) for its rollback support. Packaging: adds the trident-acl-agent.service systemd unit and RPM spec entries so the agent ships and starts on ACL images. New workspace dependencies (Cargo.toml): k8s-openapi, kube, toml - all for k8s.rs's Node watch/patch client. Split out from user/bfjelds/acl-agent-rollback-grpc for isolated review: this covers the Rust agent implementation and packaging, not the storm Go E2E test harness, test images, or pipeline definitions (stacked as a separate PR on top of this one). Verified: cargo test -p trident-acl-agent (78 passed), cargo clippy -p trident-acl-agent --all-targets -- -D warnings (clean), cargo fmt --check (clean), cargo build --workspace (clean). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86

Cargo.lock had resolved enum-ordinalize/enum-ordinalize-derive 4.4.2 (a transitive dependency via kube-runtime -> educe), which requires rustc 1.89+. The RPM build pins rust-1.86.0, causing: error: rustc 1.86.0 is not supported by the following packages: enum-ordinalize@4.4.2 requires rustc 1.89 enum-ordinalize-derive@4.4.2 requires rustc 1.89 Pinned both to 4.3.2 (rust-version 1.68, well under 1.86) via cargo update --precise. This also collapses the dependency graph back to a single syn version (2.0.90) - 4.4.2s derive crate needed syn 3.x, which no longer resolves once removed. Verified: cargo build/test/clippy -p trident-acl-agent clean, and the real docker-based "make bin/trident-rpms.tar.gz" RPM build (which uses the actual pinned rust-1.86.0 toolchain, not the newer local dev rustc that produced the original lockfile) now succeeds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86

Copilot

Pull request overview

This PR rewrites trident-acl-agent (Harpoon) into an annotation-driven ACL update agent that watches a Kubernetes Node annotation to drive Trident update/rollback/commit flows over gRPC, persists post-reboot state, and publishes status back to the Node. It also wires the agent into packaging (systemd + RPM) and adds supporting config/state/k8s client code plus unit tests (including an in-process mock tridentd).

Changes:

Add an annotation-based reconcile loop that stages/finalizes updates, stages/finalizes rollbacks, and resumes post-reboot commit using persisted state.
Introduce wire types + validation for request/status annotations, plus a gRPC client wrapper and an in-process mock tridentd for unit testing.
Package the agent with a systemd unit and RPM spec integration; expand workspace dependencies to include kube-rs/k8s-openapi/toml.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
packaging/systemd/trident-acl-agent.service	Adds systemd unit to run the ACL agent on hosts.
packaging/rpm/trident.spec	Installs/enables the new systemd unit and ships the agent binary in the RPM.
crates/trident-acl-agent/src/trident.rs	Implements the gRPC client wrapper for update/commit/rollback calls to `tridentd`.
crates/trident-acl-agent/src/state.rs	Implements persisted state store for completed operations and pending post-reboot commit.
crates/trident-acl-agent/src/orchestrator.rs	Adds the main reconcile loop (watch request annotation, drive Trident RPCs, publish status).
crates/trident-acl-agent/src/mock_tridentd.rs	Adds an in-process fake `tridentd` server for unit tests of the gRPC client/orchestrator.
crates/trident-acl-agent/src/main.rs	Adds CLI/config loading and selects between omaha-only vs label/annotation orchestration mode.
crates/trident-acl-agent/src/lib.rs	Refactors existing Harpoon/Omaha logic into library form and exposes new agent components.
crates/trident-acl-agent/src/k8s.rs	Adds a thin kube-rs wrapper for get/watch/patch of the Node object.
crates/trident-acl-agent/src/config.rs	Adds TOML config parsing with defaults and CLI override handling.
crates/trident-acl-agent/src/annotations.rs	Adds request/status schema types, semantic validation, and design-doc conformance tests.
crates/trident-acl-agent/Cargo.toml	Adds dependencies needed for the new agent and its unit tests.
Cargo.toml	Adds workspace dependency entries for kube-rs/k8s-openapi and toml.
Cargo.lock	Updates lockfile for newly introduced dependencies.

query_for_update() is a blocking call (reqwest::blocking under the hood in omaha::send), which was called directly from two async fns: run_omaha_only() and orchestrator.rs's handle_stage(). This panics with "Cannot drop a runtime in a context where blocking is not allowed" the moment a real response is received, because reqwest::blocking spins up its own inner Tokio runtime per call, which isn't safe to tear down from inside an already-running async task. Confirmed this is a real, standard-usage bug, not specific to any one code path: reproduced the panic both via the default omaha-only invocation (no flags) and would affect handle_stage() identically, since it uses the exact same call pattern. Fixed both call sites by running query_for_update() on a dedicated blocking thread via tokio::task::spawn_blocking, matching the fix already applied to the --validate-connection nebraska check on user/bfjelds/acl-agent-connection-check. Verified: cargo test -p trident-acl-agent (78 passed), cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check (clean). Reproduced the panic against a local mock Omaha server before the fix, confirmed clean exit 0 with no panic after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86

…braska.endpoint nebraska.poll_interval was declared, defaulted, parsed from TOML, and unit-tested, but never actually consumed anywhere: neither run_omaha_only() (a genuine one-shot, no internal loop) nor the label/annotation orchestrator (event-driven off the Kubernetes watch, not a timer) ever read it. There's also no companion systemd .timer unit to periodically re-invoke the agent. Removed the field, its default constant, its TOML parsing, and the corresponding test assertions/fixtures. Also added a default for nebraska.endpoint (previously None, requiring an explicit config or CLI override or the agent would fail to start). Defaults to a `.invalid`-TLD placeholder (RFC 2606, guaranteed to never resolve) until the real production endpoint is known - deployments that forget to override it fail loudly at the network layer instead of silently querying a real-looking but wrong host. The existing `ok_or_else` "no Nebraska endpoint configured" guards in run_omaha_only/ handle_stage/validate_connection are left in place as harmless defensive code, even though endpoint is now effectively always populated. Verified: cargo test -p trident-acl-agent (78 passed, including updated config parsing tests), cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check (clean). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86

…ode, rename Labels->Annotations Three related config changes: 1. Adds `nebraska.track` (defaults to DEFAULT_NEBRASKA_TRACK = "west-us") as a configurable field, threaded through run_omaha_only() and orchestrator.rs's handle_stage() instead of the hardcoded constant. 2. Swaps GoalSource's default from OmahaOnly to the annotation-driven orchestrator mode. The historical one-shot omaha-only behavior remains available as an explicit opt-out (`goal_source = "omaha-only"`), just no longer the shipping default. 3. Renames GoalSource::Labels -> GoalSource::Annotations (TOML value "labels" -> "annotations"). The old name was a holdover from an earlier design iteration that used Kubernetes labels before switching to annotations - flagged previously as a naming-drift risk since the wire protocol has used annotations for a while. Updated the handful of directly-coupled doc comments (main.rs, lib.rs's crate doc, trident.rs) that described this feature as "label protocol"/"label mode" for consistency; `k8s.rs`'s patch_node_labels (real Kubernetes label support, a separate feature) is untouched. Added doc comments on both GoalSource variants and on both match arms in main.rs explaining what each mode actually does. Verified: cargo test -p trident-acl-agent (78 passed, including new nebraska.track parsing coverage), cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check (clean). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86

Copilot

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

crates/trident-acl-agent/src/orchestrator.rs:50

The reboot path shells out via std::process::Command, bypassing the repo’s standard dependency execution wrapper (osutils::dependencies::Dependency) which provides consistent resolution/error reporting (and is already used for systemctl elsewhere in the workspace). Using the wrapper here avoids inconsistent failures and makes missing binaries/reporting uniform.

impl RebootHandle for SystemRebooter {
    fn reboot(&self) -> Result<(), anyhow::Error> {
        for candidate in [
            ("reboot", Vec::<&str>::new()),
            ("systemctl", vec!["reboot"]),

crates/trident-acl-agent/src/orchestrator.rs:12

After switching the reboot implementation away from std::process::Command, the process::Command import becomes unused and will trigger warnings (and potentially CI failures if warnings are denied).

use std::{collections::BTreeMap, process::Command};

crates/trident-acl-agent/Cargo.toml:54

hyper-util is added with an inline version in this crate’s manifest. This repo generally centralizes third-party versions in the workspace root and uses { workspace = true } in per-crate manifests (e.g. crates/osutils/Cargo.toml). Consider adding hyper-util to [workspace.dependencies] in the root Cargo.toml and switching this entry to { workspace = true } to keep dependency versions consistent across the workspace.

hyper-util = { version = "0.1", features = ["tokio"] }

crates/trident-acl-agent/src/annotations.rs:113

UpdateRequest::validate() claims to enforce the request annotation's formal JSON Schema, but it does not validate that operation_id is a UUID. Since operationId is specified as format: uuid / UUID regex in the embedded schema and is used as a key in caches/status mapping, invalid values should be rejected early to avoid mismatched/deduped operations.

    pub fn validate(self) -> Result<Self, String> {
        if self.schema_version != SCHEMA_VERSION {
            return Err(format!("unsupported schemaVersion {}", self.schema_version));
        }
        match self.operation {

- SystemRebooter now routes through osutils::dependencies::Dependency instead of raw process::Command, matching the rest of the codebase (crates/trident/src/reboot.rs uses the same pattern) and getting uniform actionable errors on a missing/failing systemctl - host_configuration_from_image now builds YAML via serde_yaml instead of format!, avoiding malformed/misinterpreted YAML if a URL or hash contains YAML-special characters - from_toml error message now names trident-acl-agent.conf instead of the generic "config.toml" - CURRENT_VERSION_STUB changed to a sentinel that cannot collide with a real AKS/Trident release version string, preventing a spurious AlreadyAtTarget short-circuit - StateStore::save now writes to a temp file and renames atomically instead of truncate-then-write, so a crash/power-loss around a real reboot cannot corrupt state.json - hyper-util moved to [workspace.dependencies] and referenced via workspace = true, matching repo convention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86

Copilot

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/trident-acl-agent/src/k8s.rs:110

watch_node() sets the Kubernetes watch request timeoutSeconds to watch_poll_interval (default 2s). That causes the watch connection to be intentionally torn down and re-established every couple seconds even when healthy, increasing API-server load and generating unnecessary reconnect churn (contradicting the comment that this is a backoff ceiling). Use a longer, fixed watch timeout (or omit it) and keep watch_poll_interval for backoff/retry behavior instead.

        let watcher_config = watcher::Config::default()
            .fields(&format!("metadata.name={name}"))
            .timeout(self.poll_interval.as_secs().max(1) as u32);

crates/trident-acl-agent/src/main.rs:64

is_network_target() allocates a new String for every log record (format!("{prefix}::") inside any(...)). This runs on the hot path of log filtering and can add noticeable overhead under verbose logging. Use strip_prefix/starts_with("::") to check module prefixes without allocation.

fn is_network_target(target: &str) -> bool {
    NETWORK_LOG_TARGETS
        .iter()
        .any(|prefix| target == *prefix || target.starts_with(&format!("{prefix}::")))
}

crates/trident-acl-agent/src/trident.rs:366

consume_servicing_stream() builds an owned String for every streamed log record (format!(...)), even when the selected log level is disabled. Since servicing streams can be chatty, this creates avoidable allocation overhead. Log with format args directly so the formatting only occurs when the level is enabled.

            Some(ResponseBody::Log(log_record)) => {
                let msg = format!("[Trident:{operation}] {}", log_record.message);
                match log_record.level() {
                    LogLevel::Unspecified | LogLevel::Trace => log::trace!("{msg}"),
                    LogLevel::Debug => log::debug!("{msg}"),

bfjelds · 2026-08-06T00:24:15Z

+    /// exit. No Kubernetes involvement at all - no annotations, no watch,
+    /// no Node access. Kept as an explicit opt-out for nodes that don't
+    /// participate in the AKS annotation-driven update protocol.
+    OmahaOnly,


this is the previously existing behavior, maybe we get rid of it (and its backing code) to simplify?

This was referenced Aug 5, 2026

trident-acl-agent: add storm trident-acl-agent E2E test harness, test images, and pipeline #731

Draft

trident: promote rollback to stable v1; adjust rollback return type to detect no-servicing #729

Open

bfjelds force-pushed the user/bfjelds/rollback-grpc-promotion branch from 057a7ae to d27e576 Compare August 5, 2026 20:30

bfjelds and others added 2 commits August 5, 2026 20:32

bfjelds force-pushed the user/bfjelds/acl-agent-rollback-grpc-rust branch from 6598fdd to c3dfd8d Compare August 5, 2026 20:34

bfjelds requested a lite review from Copilot August 5, 2026 20:59

Copilot started reviewing on behalf of bfjelds August 5, 2026 21:00 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

bfjelds and others added 3 commits August 5, 2026 21:43

bfjelds requested a lite review from Copilot August 5, 2026 22:34

Copilot started reviewing on behalf of bfjelds August 5, 2026 22:35 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

bfjelds requested a lite review from Copilot August 5, 2026 23:40

Copilot started reviewing on behalf of bfjelds August 5, 2026 23:41 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

bfjelds commented Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

trident-acl-agent: rewrite as annotation-based ACL update agent - #730

trident-acl-agent: rewrite as annotation-based ACL update agent#730
bfjelds wants to merge 6 commits into
user/bfjelds/rollback-grpc-promotionfrom
user/bfjelds/acl-agent-rollback-grpc-rust

bfjelds commented Aug 5, 2026 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Aug 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

bfjelds Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bfjelds commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Validation

PR details

Packaging

Uh oh!

azure-pipelines Bot commented Aug 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

bfjelds Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bfjelds commented Aug 5, 2026 •

edited

Loading